-
Notifications
You must be signed in to change notification settings - Fork 9
Improve the stress scripts #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
holly-cummins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to stress.sh make a lot of sense, and I think are on the right side of the 'understandability vs robustness' tradeoff.
For stress-all.sh, what's the reasoning for duplicating stress.sh rather than just calling out to it? Is it to be able to redirect just parts of it to a file?
|
I mostly kept them separate at this point as I assumed that you probably wouldn't want to merge If you think you'd like to have both I can make sure that they share functions and tidy up a little. |
| wait_for_8080() { | ||
| echo "Waiting for port 8080..." | ||
| for ((i=0; i<30; i++)); do | ||
| # Using 127.0.0.1 is safer than localhost on macOS to avoid IPv6 ::1 mismatch | ||
| if (echo > /dev/tcp/127.0.0.1/8080) >/dev/null 2>&1; then | ||
| return 0 | ||
| fi | ||
| sleep 1 | ||
| done | ||
| echo "Timeout waiting for port 8080" | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quintesse have a look also at this script, could be useful for your own; this one waits just enough for the frameworks to have opened port 8080. Seems like a good replacement for the "10 seconds" rule.
Also, you could then use the total time of the benchmark - including boostrap times - as a fair comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Delawen as well FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sanne !
Let's bring them all. Otherwise i suspect we might end up locally re-inventing that capability. :) |
No description provided.